Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Large Gruvbox refactoring #10773

Merged

Conversation

Chirikumbrah
Copy link
Contributor

@Chirikumbrah Chirikumbrah commented May 16, 2024

Not so long ago I completely switched from Dracula to Gruvbox and I decided to make it more comfortable according to NVim Gruvbox colors.

I changed operator color with some other colors and disabled some bold fonts:
image

I also improved cursor/cursorline colors and made cursor color the same as the mode color. You can notice that primary cursorline is a bit lighter:

  • Normal mode with multicursor:
    image

  • Selection mode with multicursor:
    image

  • Insert mode with multicursor:
    image


Now, diagnostics:
I decided to choose diagnostics colours according to their severity level:

  • Hint - aqua
    image

  • Info - yellow
    Sorry for no image but I didn't find any examples :)

  • Warning - orange and Error - red
    image


I hope you will like theese changes and I am open to your improvements :)

@Chirikumbrah Chirikumbrah changed the title Large Gruvbox refactoring. Large Gruvbox refactoring May 16, 2024
@justinesmithies
Copy link

Nice ! I use gruvbox and that is an improvement indeed.
Fingers crossed it's accepted.

@Locorock
Copy link

Locorock commented May 16, 2024

A few things that left me puzzled about this:

Now for some personal opinions:

  • diagnostics for hints/warnings have mismatched colors
  • dimming text for warning makes spotting errors really hard, this is even worse for the light theme
  • rust variables end up getting colored when used in expressions, not sure if this is intended or not
  • operators being the same color of operands makes code less legible
  • there are also some oddities with diagnostic priority which i wasn't able to pinpoint, not sure if this is the theme's fault

grim-2024-mag-16-15_33_57

grim-2024-mag-16-15_48_28

@Chirikumbrah
Copy link
Contributor Author

How does this affect downstream themes like gruvbox light? Also i'm pretty sure you should merge #10506

I think orange is more suitable for warnings and yellow for infos, so I will change colors there.

@Locorock
Copy link

How does this affect downstream themes like gruvbox light? Also i'm pretty sure you should merge #10506

I think orange is more suitable for warnings and yellow for infos, so I will change colors there.

that would be the case but red and orange are quite hard to differentiate in gruvbox (especially with the light theme)

@Chirikumbrah
Copy link
Contributor Author

  • operators being the same color of operands makes code less legible

I brought the purple1 back)

@Chirikumbrah
Copy link
Contributor Author

Chirikumbrah commented May 18, 2024

  • if you're basing this off neovim gruvbox, why are you removing bold functions?

Because I didn't notice that functions are bold in the gruvbox.nvim and for me it is not so convinient. NeoVim's example below)
image

@Chirikumbrah
Copy link
Contributor Author

  • dimming text for warning makes spotting errors really hard, this is even worse for the light theme

Only unnecessary objects are dimmed)
image

  • diagnostics for hints/warnings have mismatched colors
  • rust variables end up getting colored when used in expressions, not sure if this is intended or not
  • there are also some oddities with diagnostic priority which i wasn't able to pinpoint, not sure if this is the theme's fault

I agree, but could you please suggest some better solutions for some colors?
To be honest I don't know the better way to prioritize diagnostics colors, but yes, in the light themes orange and red are quite the same.

And thank you for your participation :)

@Chirikumbrah
Copy link
Contributor Author

The main question for me in this case is will it be ok if I set yellow for warnings and aqua for both hints and infos?)

@kirawi kirawi added C-enhancement Category: Improvements A-theme Area: Theme and appearence related labels May 18, 2024
@Locorock
Copy link

Locorock commented May 18, 2024

I agree, but could you please suggest some better solutions for some colors? To be honest I don't know the better way to prioritize diagnostics colors, but yes, in the light themes orange and red are quite the same.

I think the current diagnostic colors are fine:

  • Red for errors
  • Yellow for warnings
  • Aqua for hints
  • Blue for info

Now the real question becomes how do we handle diagnostic information which is not in the original theme, namely Unnecessary and Deprecated:

  • Deprecated seems straightforward enough, strikethrough seems like a standard at this point
  • Unnecessary is a bit more complex, adding dimming might be fine but, talking about gutter dots here, helix considers unnecessary diagnostics as warnings, so the colors are always going to be mismatched. Realistically speaking i think we're better off leaving unnecessary diagnostics with the default behaviour, at least until helix handles gutter diagnostics correctly.

@Chirikumbrah
Copy link
Contributor Author

Chirikumbrah commented May 18, 2024

  • Red for errors
  • Yellow for warnings
  • Aqua for hints
  • Blue for info

Made so, but aqua for info and blue for hints as it was before :)

  • i think we're better off leaving unnecessary diagnostics with the default behaviour

I commented this line for future changes.

Probably we should, by the way, but I used gruvbox theme only with NeoVim. Not Vim. So for me it's easier to compare this theme with NeoVim's implementation.
If you have any additional suggestions, I'd be happy to discuss them)

@Chirikumbrah
Copy link
Contributor Author

@Locorock, I also set diff.delta color to yellow as for the color of warnings :)

@Chirikumbrah Chirikumbrah force-pushed the gruvbox-theme-refactoring branch from 92ddffa to 6caf005 Compare May 20, 2024 06:12
@Chirikumbrah Chirikumbrah force-pushed the gruvbox-theme-refactoring branch from 9c1e3fd to 6caf005 Compare June 15, 2024 12:15
@Chirikumbrah
Copy link
Contributor Author

Chirikumbrah commented Jun 15, 2024

@archseer, @kirawi
What do you think about my PR?)
Merge please when you'll have a time if everything is fine :)

@Chirikumbrah Chirikumbrah force-pushed the gruvbox-theme-refactoring branch from 38fbf9e to 9069411 Compare June 21, 2024 05:01
@pascalkuthe pascalkuthe merged commit b4811f7 into helix-editor:master Jun 26, 2024
6 checks passed
@Chirikumbrah Chirikumbrah deleted the gruvbox-theme-refactoring branch June 30, 2024 12:34
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
* gruvbox refactoring

* removed unnecessary lines

* set purple1 for operators

* changed diagnostics colors

* removed some unnecessary lines

* set diff.delta color to yellow

* removed some tag colors
mxxntype pushed a commit to mxxntype/helix that referenced this pull request Aug 14, 2024
* gruvbox refactoring

* removed unnecessary lines

* set purple1 for operators

* changed diagnostics colors

* removed some unnecessary lines

* set diff.delta color to yellow

* removed some tag colors
kyruzic pushed a commit to kyruzic/helix that referenced this pull request Sep 27, 2024
* gruvbox refactoring

* removed unnecessary lines

* set purple1 for operators

* changed diagnostics colors

* removed some unnecessary lines

* set diff.delta color to yellow

* removed some tag colors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-theme Area: Theme and appearence related C-enhancement Category: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants